inject() should accept bound variables - #3636
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3636 +/- ##
============================================
+ Coverage 76.35% 76.67% +0.31%
- Complexity 13424 14328 +904
============================================
Files 1012 1037 +25
Lines 60341 64759 +4418
Branches 7075 7694 +619
============================================
+ Hits 46076 49656 +3580
- Misses 11548 12010 +462
- Partials 2717 3093 +376 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
98d4d2e to
37a4162
Compare
|
How do you get code coverage to rerun? |
inject() was the only variadic-generic step whose grammar used genericLiteralVarargs rather than genericArgumentVarargs, so it rejected a bound variable that every sibling step (V, E, within, without, hasId, hasValue, property) already accepts. Change both inject productions to genericArgumentVarargs and route the two inject visitors through ArgumentVisitor.parseObjectVarargs, mirroring V()/E(). Adapt the Groovy translators (Java and JavaScript) to the flatter varargs tree shape. This is a strict superset: genericArgument includes genericLiteral, so every existing inject(...) call is unchanged and only inject(<variable>) becomes newly valid.
37a4162 to
d45b885
Compare
|
hello and thanks for the contribution. i was wondering if you had a specific need/use case for this feature that you could share or if it was just a point of syntax consistency that was driving it. any details you could share with us on that? |
|
Actually it's a super cool feature (well I think so), just by adding
support for this into the grammar we get federated graph calls.
I am building a clean room Gremlin server where each graph is fully
isolated. To query across graphs we have a federate step that runs a
sub-traversal on a sibling graph. The hard case is a mid-traversal federate:
every traverser on the local side carries a value that has to reach the
remote side, and we very much do not want a round trip per traverser.
So we batch them at the barrier. All the parents cross as one {parentId:
parentValue} map, a single bound parameter, and the remote side runs plain
Gremlin over it:
inject($map).unfold().group().by(Column.keys).by(<per-parent sub-traversal
over the entry value>)
That is the entire transport. group().by(Column.keys) gives us correlation
for free, and it stays correct when two different parents inject equal
values, because they are still two distinct keys. Results come back keyed
by parent, so we scatter them locally. The remote server needs no knowledge
whatsoever that it is being federated to. It is just a Gremlin query with a
bound parameter.
What the user writes is standard as well. An alias before the boundary, a
select after it:
g.V().hasLabel("person").values("name").as("e")
.call("federate", ["graph": "crew"])
.V().has("name", select("e"))
The one thing blocking it was inject() taking genericLiteralVarargs, so the
map could not arrive as a parameter at all. Hence the patch. Passing it as
a single bound map rather than per-parent arguments also matters more than
it looks: Our runtime caps a statement at 100 bound parameters, so we have
to make sure we don't hit it
The reason we're keen on it is where it goes next. Once the remote side can
return vertices and edges rather than only values and counts, federation is
complete. You can pull a subgraph from another graph and keep traversing
it, and since it is all standard Gremlin over HTTP, that works across
implementations, not just between two of ours.
|
|
Hi @danielbodart, thanks for the submission and the response. Your federated queries use case sounds quite interesting, although there may be a connection there that I am missing. I'm not quite understanding why the map cannot be inlined into the InjectStep as a The entire purpose of passing With this in mind, supporting variables in a step is actually a much more involved process than simply enabling the functionality in the grammar and parser. There needs to be an ability for the variables to be preserved inside the parsed The preservation of GValues in the parser is controlled by the The set of steps which are currently permitted to accept variables was carefully curated to target steps which showed the greatest need and upside for this query caching use case. Due to the complexity of the |
|
Interesting, so the reason I can't inline them is because the values from the parent traversal can contain whole sub graphs of data that are projected into the sibling graph (so can be pretty big / unbounded) and exactly as you say I can now benefit from query caching. My implementation uses SQLlitre as the backing store and any bindings just become prepare statement bindings 1-2-1 with no extra complexity introduced. So inlining (for my implementation) can actually stop the query running (I lower the whole gremlin traversal into a relational algebra and then compile to SQL) and with unbound inlining of a graph that could easily hit the max statement size of SQLlite, it's not cachable as you already mentioned but it's also a lot slower to parse as the map would need to be parsed by gremlin parser (antlr-ng in my case) rather than as native code inside SQLite. But I am a realist and understand none of these are your concerns! I didn't realise the inconsistency was intentional and though this was an easy win for everyone and just wanted to upstream my patch. I can carry on shipping an enhanced gremlin library for the federated use cases. I'll close this but have a few more upstream patches I'd like to see if any of them are interest to you all |
4 participants
What
inject()is the only variadic-generic step in the Gremlin grammar that rejects a bound variable. This changes bothinjectproductions fromgenericLiteralVarargstogenericArgumentVarargs, soinject(<variable>)parses and resolves like every sibling step.Why (consistency)
A
…Argumentrule is exactlyliteral | variable— thevariablealternative is the bound parameter. Counting who uses which variadic-generic rule inGremlin.g4:genericArgumentVarargs(accepts a bound variable) is used byV(),E()(spawn and mid-traversal),hasId(),hasValue(),property(), and — the closest structural twin —within()/without().genericLiteralVarargs(literal-only) is referenced by nothing but the twoinjectproductions.So
injectis the lone holdout: its nearest siblingwithin(), which does the identical "spray a list of values in" job, already accepts a bound parameter. This alignsinjectwith the rest of the language rather than adding anything new to it.Compatibility
Strict superset.
genericArgumentincludesgenericLiteral(maps included), so every existinginject(...)call parses and behaves exactly as before; only the previously-rejectedinject(<variable>)becomes valid. Verified against the existing negative grammar corpus (incorrect-gremlin-values.txt) — it contains no bare identifiers, so nothing moves from "correctly rejected" to "now accepted."Changes
Gremlin.g4): bothinjectproductions →genericArgumentVarargs.TraversalSourceSpawnMethodVisitor,TraversalMethodVisitor): route throughArgumentVisitor.parseObjectVarargs(ctx.genericArgumentVarargs()), identical to howV()/E()already work.inject(x, null)Groovy-closure disambiguation walked agenericLiteralExprlayer thatgenericArgumentVarargsdoesn't have (the args sit directly under the varargs node); adapted to the flatter shape. Output is unchanged for all existing cases includinginject(1, null).g.inject(x)/g.V().inject(x)(BasicGrammarTest), and an end-to-end variable-resolution assertion inGremlinQueryParserTest.shouldParseVariablesInVarargsmirroring the existingg.V(x, y, 300)case.The now-orphaned
genericLiteralVarargsrule is left in place (harmless; its context class is still generated, keepingGenericLiteralVisitor/DefaultGremlinBaseVisitorcompiling) — happy to remove it if preferred.Notes
🤖 Generated with Claude Code